Skip to content

fix(clickhouse): escape SQL identifiers and literals in metadata/DDL paths (#1914) - #2174

Merged
openai0229 merged 6 commits into
OtterMind:mainfrom
HandSonic:fix/sqli-clickhouse
Jul 29, 2026
Merged

fix(clickhouse): escape SQL identifiers and literals in metadata/DDL paths (#1914)#2174
openai0229 merged 6 commits into
OtterMind:mainfrom
HandSonic:fix/sqli-clickhouse

Conversation

@HandSonic

@HandSonic HandSonic commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Related issue

Part of #1914. This PR handles the ClickHouse plugin.

Summary

  • Add ClickHouseIdentifierProcessor as the plugin's identifier and string-literal processor. Conditional SPI quoting remains conditional, generated DDL uses an explicit always-quote path, embedded backticks round-trip correctly, and reserved-word matching is locale-independent.
  • Route ClickHouse metadata, DDL builder, and manager paths through the dialect processor. Generated table names use ClickHouse's database.table form, prefer schemaName, and consistently qualify drop, truncate, and copy operations.
  • Validate column types, table engines, and default expressions with quote-aware balanced scanners. Legal nested ClickHouse syntax such as Enum8('a' = -1), DateTime64(3, 'Asia/Shanghai'), Distributed(cluster, db, table, cityHash64(id)), and toDateTime64(now(), 3) is preserved without re-encoding serialized literals.
  • Keep parameterized types in ALTER TABLE, emit valid rename/modify actions, and map schema create/drop operations to ClickHouse CREATE/DROP DATABASE.
  • Add focused regression coverage for identifier round trips, qualified names, DDL generation, legal expressions, malformed expression rejection, and inherited builder paths.

Affected surfaces

  • Frontend / Web
  • Backend / API / Storage
  • Database plugin / Driver
  • JCEF / Desktop packaging
  • CI / Build / Release
  • Documentation only

Verification

  • Commands and results: rtk mvn -B -pl chat2db-community-plugins/chat2db-community-clickhouse -am -f chat2db-community-server/pom.xml -Dmaven.test.skip=false -DskipTests=false -Dsurefire.failIfNoSpecifiedTests=false -Dmaven.test.failure.ignore=false clean test completed with BUILD SUCCESS across all 9 reactor modules. ClickHouse tests: 43 passed; MySQL dependency-module tests: 653 passed; failures/errors/skips: 0.
  • Manual verification: exact generated SQL assertions cover embedded backticks, schema-qualified copy/drop/truncate, parameterized ALTER TABLE types, nested engine/default expressions, quoted literals, and rejection of unbalanced or multi-statement expressions.
  • UI evidence: N/A

Risk and compatibility

  • Public API or stored data: N/A. The change is isolated to generated ClickHouse SQL and the plugin's existing SPI implementation.
  • Database or driver compatibility: legal ClickHouse type, engine, and default-expression syntax is preserved; names are emitted as backtick-quoted ClickHouse identifiers.
  • Network, privacy, or security: no network or privacy behavior changes. Malformed metadata-derived SQL fragments are rejected instead of being emitted.
  • Community / Local / Pro boundary: Community ClickHouse plugin only.
  • Backward compatibility: ordinary object names remain compatible; schema-qualified DDL is now consistent, and previously malformed SQL for names containing backticks or parameterized types is corrected.

Reviewer map

  • Start here: ClickHouseIdentifierProcessor, ClickHouseSqlGuards, ClickHouseSqlBuilder, and ClickHouseIdentifierProcessorTest.
  • Failure condition: an identifier is double-quoted or loses embedded backticks, a legal nested ClickHouse expression is rejected or rewritten, or generated DDL omits/misqualifies a table name or parameterized type.
  • Rollback or disable path: revert this PR; there is no runtime feature flag.

Contributor declaration

  • I linked the Issue that defines this change.
  • I tested the affected behavior and reported the actual results above.
  • I did not include credentials, private data, or generated build output.
  • I disclosed substantial AI assistance below, or this PR contains no substantial AI-generated code.

AI assistance: Maintainer review, implementation refinements, test expansion, and PR description updates used AI coding assistance.

@HandSonic
HandSonic requested a review from openai0229 as a code owner July 26, 2026 09:53
@openai0229 openai0229 moved this to In Review in Chat2DB Community Jul 26, 2026
@HandSonic
HandSonic marked this pull request as draft July 26, 2026 09:56
@HandSonic

Copy link
Copy Markdown
Contributor Author

Converting to draft: final adversarial review pass is still in progress for this branch. Will mark ready for review once it completes. (process note: PR was opened prematurely by automation)

@openai0229 openai0229 moved this from In Review to In Progress in Chat2DB Community Jul 26, 2026
@HandSonic
HandSonic force-pushed the fix/sqli-clickhouse branch 2 times, most recently from 8ac520b to 9bcd7e9 Compare July 26, 2026 10:19
@HandSonic
HandSonic marked this pull request as ready for review July 26, 2026 10:28
@openai0229 openai0229 moved this from In Progress to In Review in Chat2DB Community Jul 26, 2026
@openai0229
openai0229 force-pushed the fix/sqli-clickhouse branch from c8332c7 to 5684d01 Compare July 29, 2026 07:28

@openai0229 openai0229 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maintainer review complete. Identifier round-trip, expression guards, qualified DDL paths, and parameterized ALTER behavior are covered by the 43-test ClickHouse suite; the full 9-module reactor passes.

HandSonic and others added 6 commits July 29, 2026 15:37
…r per maintainer review (OtterMind#1914)

- new ClickHouseIdentifierProcessor (SPI ISQLIdentifierProcessor): quoteIdentifier
  with backtick doubling, escapeString with backslash + single-quote doubling
- ClickHouseMetaData overrides getSQLIdentifierProcessor(); metadata call sites use it
- builders/managers/enums use ClickHouseIdentifierProcessor.INSTANCE
- non-escapable validation moved to ClickHouseSqlGuards (column type expressions,
  engine whitelist, column default expressions)
- ClickHouseSqlEscapes removed; tests migrated (24 green)
…-quote for DDL paths (OtterMind#1914)

- quoteIdentifier(String) is conditional again: null/blank pass through,
  plain non-keyword identifiers stay unquoted, everything else is wrapped
  in backticks with embedded backticks doubled; versioned overload delegates
- new quoteIdentifierAlways(String) carries the old unconditional semantics
  for DDL-generation call sites migrated from ClickHouseSqlEscapes
- quoteIdentifierIgnoreCase stays the always-quote, case-preserving variant
- override removeIdentifierQuote/isQuoteIdentifier so backtick-quoted
  identifiers round-trip through completion/matching consumers
- add ClickHouse reserved keyword set used by the conditional check
- tests cover conditional vs always behavior incl. null passthrough (30 green)
@openai0229
openai0229 force-pushed the fix/sqli-clickhouse branch from 5684d01 to f73da33 Compare July 29, 2026 07:38
@openai0229
openai0229 merged commit 6186b8c into OtterMind:main Jul 29, 2026
16 checks passed
@openai0229 openai0229 moved this from In Review to Done in Chat2DB Community Jul 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants